Skip to content
This repository has been archived by the owner on Jan 23, 2024. It is now read-only.

apm-ui: use the new kibana project layout #769

Merged
merged 4 commits into from
Feb 21, 2020

Conversation

v1v
Copy link
Member

@v1v v1v commented Feb 21, 2020

What does this PR do?

Fixes the current filesystem layout issue with the kibana project.
Reduce the verbose output in the clone
Fixes some linting issues.

Why is it important?

Fix the apm-its

Related issues

Leftovers from elastic/kibana#57532
Closes #770

Questions

  • Do we need to support backward compatibility?
  • Do we need to backport this to the *.x branches?

@v1v v1v self-assigned this Feb 21, 2020
@v1v v1v added the automation label Feb 21, 2020
@v1v v1v requested review from a team, dgieselaar and sorenlouv February 21, 2020 12:06
@@ -1,9 +1,12 @@
#!/usr/bin/env bash
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix a lint defect

cd ./tmp
git clone --depth 1 -b $BRANCH https://github.com/$OWNER/kibana.git
mv ./kibana/x-pack/legacy/plugins/apm/typings/es_schemas ./apm-ui-interfaces
cd ./tmp || true
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix a lint defect

Copy link
Member

@sorenlouv sorenlouv Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do? Is it if it fails navigating to ./tmp we want to continue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, have you got a different suggestion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering why this would fail - and fix the root cause of that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll keep as it was then

Copy link
Member Author

@v1v v1v Feb 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rollback my changes, to keep it as it was

git clone --depth 1 -b $BRANCH https://github.com/$OWNER/kibana.git
mv ./kibana/x-pack/legacy/plugins/apm/typings/es_schemas ./apm-ui-interfaces
cd ./tmp || true
git clone --quiet --depth 1 -b "${BRANCH}" "https://github.com/${OWNER}/kibana.git"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • reduce verbose output in the CI.
  • fix a lint defect

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally made this git clone a little more complicated than it has to be. What I wanted was a partial checkout of just a specific path (to make it faster). I didn't find any simple ways to do that.
I don't think I saw any perf improvements by using the --depth flag (but not 100%)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you don't have to change this. Just thought I'd mention it.

mv ./kibana/x-pack/legacy/plugins/apm/typings/es_schemas ./apm-ui-interfaces
cd ./tmp || true
git clone --quiet --depth 1 -b "${BRANCH}" "https://github.com/${OWNER}/kibana.git"
mv ./kibana/x-pack/plugins/apm/typings/es_schemas ./apm-ui-interfaces
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The real change!
Question:

  • Do we need to support backward compatibility?

if so

newLocation=./kibana/x-pack/plugins/apm/typings/es_schemas
location=./kibana/x-pack/legacy/plugins/apm/typings/es_schemas
if [ -d "${newLocation}" ] ; then
   location=${newLocation}
fi
mv "${location} ./apm-ui-interfaces

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. If the above works I think that's a good approach. Perhaps add a comment saying that in 7.7 files moved around. Then we can move this again in a couple of minors from now.

@v1v v1v marked this pull request as ready for review February 21, 2020 12:14
Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing 👍

@v1v v1v merged commit 5d5fc26 into elastic:master Feb 21, 2020
v1v added a commit to v1v/apm-integration-testing that referenced this pull request Feb 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[apm-ui] Tests are broken since a recent change in the Kibana apm-ui layout
4 participants